feat: Async payment migration with transactional safety, REST API, and comprehensive tests#180
Conversation
…sive tests Phase 1: Added @transactional to deposit(), withdraw(), and transferAmount() Phase 2: Async migration with AsyncConfig, TransactionLoggingService, BankRestController (REST API), TransactionStatus enum Phase 3: 40 tests (unit + integration + async + transactional integrity) Phase 4: LEARNINGS.md documenting @async, @transactional, CompletableFuture, race conditions, and SecurityContext thread boundaries Co-Authored-By: Joao Esteves <joao.esteves@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Devin Review
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| toAccount.setBalance(toAccount.getBalance().add(amount)); | ||
| accountRepository.save(toAccount); | ||
|
|
||
| // Create transaction records for both accounts | ||
| Transaction debitTransaction = new Transaction( | ||
| amount, | ||
| "Transfer Out to " + toAccount.getUsername(), | ||
| LocalDateTime.now(), | ||
| fromAccount | ||
| ); | ||
| transactionRepository.save(debitTransaction); | ||
|
|
||
| Transaction creditTransaction = new Transaction( | ||
| amount, | ||
| "Transfer In from " + fromAccount.getUsername(), | ||
| LocalDateTime.now(), | ||
| toAccount | ||
| ); | ||
| transactionRepository.save(creditTransaction); | ||
| // Delegate transaction logging to async service | ||
| transactionLoggingService.logTransferAsync( | ||
| fromAccount, amount, "Transfer Out to " + toAccount.getUsername()); | ||
| transactionLoggingService.logTransferAsync( | ||
| toAccount, amount, "Transfer In from " + fromAccount.getUsername()); | ||
| } |
There was a problem hiding this comment.
🔴 Stale entity read outside @transactional boundary enables lost updates and double-spending
In BankRestController, the Account entity is loaded via accountService.findAccountByUsername(username) (which is NOT @Transactional) BEFORE the @Transactional service methods (deposit(), withdraw(), transferAmount()) are called. The balance check and balance mutation inside those methods use the in-memory balance from this stale, detached entity.
For transferAmount() specifically (AccountService.java:97-118): fromAccount is passed in from the controller with a potentially stale balance. Two concurrent transfers can both read $1000, both pass fromAccount.getBalance().compareTo(amount) < 0, both subtract, and when accountRepository.save(fromAccount) calls merge(), the stale balance overwrites any concurrent committed changes. This allows transferring more money than available (double-spending).
Concrete race scenario
Thread A reads balance=$1000, Thread B reads balance=$1000.
Thread A enters transferAmount, subtracts $700 → saves $300, commits.
Thread B enters transferAmount, checks stale $1000 >= $700 ✓, subtracts $700 → merge overwrites to $300, commits.
Result: $1400 transferred out but only $700 deducted. Money was created from nothing.
The @Transactional provides atomicity (both account saves succeed or fail together) but does NOT prevent lost updates because merge() copies the detached entity's stale field values over the freshly-loaded managed entity. Without @Version (optimistic locking) or loading the entity inside the transaction, this race condition remains exploitable.
(Refers to lines 96-118)
Prompt for agents
The root cause is that Account entities are loaded outside the @Transactional boundary in both BankController and BankRestController, then passed into @Transactional service methods. The service methods mutate the stale in-memory balance and merge it back, overwriting any concurrent committed changes.
To fix this, the @Transactional service methods should re-load the Account entity from the database inside the transaction, rather than trusting the stale entity passed as a parameter. For example, in transferAmount(), instead of using fromAccount.getBalance() directly, reload it: Account freshFrom = accountRepository.findById(fromAccount.getId()).orElseThrow(...); then operate on freshFrom. Alternatively, add @Version to the Account entity for optimistic locking, which would cause concurrent merges to throw OptimisticLockException. The same fix applies to deposit() and withdraw() in AccountService.java.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 1216a1e. All three @Transactional methods (deposit, withdraw, transferAmount) now reload the Account from the database via accountRepository.findById() at the start of the transaction, ensuring the balance check and mutation operate on the current committed state rather than a stale detached entity passed from the controller.
| return CompletableFuture.supplyAsync(() -> { | ||
| Account account = accountService.findAccountByUsername(username); | ||
| accountService.deposit(account, amount); | ||
|
|
||
| Account updated = accountService.findAccountByUsername(username); | ||
| Map<String, Object> response = new HashMap<>(); | ||
| response.put("message", "Deposit successful"); | ||
| response.put("newBalance", updated.getBalance()); | ||
| response.put("amount", amount); | ||
| return ResponseEntity.ok(response); | ||
| }, paymentTaskExecutor); |
There was a problem hiding this comment.
🟡 Deposit REST endpoint missing exception handling unlike withdraw and transfer
The deposit() endpoint in BankRestController does not wrap the service call in a try-catch block, unlike withdraw() (line 56-61) and transfer() (line 80-86) which catch RuntimeException and return a proper 400 response with an error message. If accountService.deposit() throws any RuntimeException (e.g., a database constraint violation, or a negative amount causing downstream issues), the CompletableFuture completes exceptionally and Spring returns an opaque 500 error instead of a structured error response.
| return CompletableFuture.supplyAsync(() -> { | |
| Account account = accountService.findAccountByUsername(username); | |
| accountService.deposit(account, amount); | |
| Account updated = accountService.findAccountByUsername(username); | |
| Map<String, Object> response = new HashMap<>(); | |
| response.put("message", "Deposit successful"); | |
| response.put("newBalance", updated.getBalance()); | |
| response.put("amount", amount); | |
| return ResponseEntity.ok(response); | |
| }, paymentTaskExecutor); | |
| return CompletableFuture.supplyAsync(() -> { | |
| Account account = accountService.findAccountByUsername(username); | |
| try { | |
| accountService.deposit(account, amount); | |
| } catch (RuntimeException e) { | |
| Map<String, Object> errorResponse = new HashMap<>(); | |
| errorResponse.put("error", e.getMessage()); | |
| return ResponseEntity.badRequest().body(errorResponse); | |
| } | |
| Account updated = accountService.findAccountByUsername(username); | |
| Map<String, Object> response = new HashMap<>(); | |
| response.put("message", "Deposit successful"); | |
| response.put("newBalance", updated.getBalance()); | |
| response.put("amount", amount); | |
| return ResponseEntity.ok(response); | |
| }, paymentTaskExecutor); |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 1216a1e. Added try-catch around accountService.deposit() matching the pattern used in withdraw() and transfer() endpoints.
…pdates - Reload Account from DB via findById() inside deposit(), withdraw(), and transferAmount() to prevent stale entity reads and double-spending - Add try-catch error handling to deposit REST endpoint for consistency with withdraw and transfer endpoints - Update unit tests to mock findById() calls Co-Authored-By: Joao Esteves <joao.esteves@cognition.ai>
Summary
Migrates synchronous payment flows (deposit, withdraw, transfer) to use async patterns, adds transactional safety, introduces a REST API layer, and generates comprehensive tests for all critical payment flows.
Key changes:
@Transactionalondeposit(),withdraw(),transferAmount()with fresh entity reload inside transaction boundary to prevent stale reads/double-spendingTransactionLoggingServicewith@Async("paymentTaskExecutor")for non-blocking audit trailBankRestControllerREST API at/api/**with async endpointsTransactionStatusenum (PENDING,COMPLETED,FAILED)docs/LEARNINGS.mddocumenting Spring async/transactional patternsReview & Testing Checklist for Human
AccountService.deposit(),withdraw(), andtransferAmount()— each method callsaccountRepository.findById()at the start to get a fresh entity within the transaction boundary. Confirm this prevents the stale-entity race condition described in the review commentsPOST /api/transfer) to verify no double-spending occurs under load — thefindById()reload mitigates but does not use pessimistic/optimistic locking, so extreme concurrency may still need@VersiononAccountTransactionLoggingServicelogs errors but doesn't retry. Check if missing audit records are acceptable or if a retry/dead-letter mechanism is neededNotes
Transactionentity table was renamed tobank_transactionto avoid SQL reserved keyword conflicts with H2 (and for safety with other databases)SecurityContextHolderdoesn't propagate to async threads — username is captured synchronously beforeCompletableFuture.supplyAsync()Link to Devin session: https://app.devin.ai/sessions/625f881cdd794bfb84c645333588e44b
Requested by: @joao-cognition